Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move api helpers.go to a subpackage #44296

Merged

Conversation

caesarxuchao
Copy link
Member

Part of #44065.

This PR moves the pkg/api/helpers.go to its own subpackage. It's mostly a mechanic move, except that

  • I removed ConversionError in helpers.go, it's not used by anyone
  • I moved the 3 methods of Taint and Toleration to pkg/api/methods.go, and left a TODO saying refactoring these methods to functions.

I'll send a few more PRs to make the k8s.io/kubernetes/pkg/api package only contains the code we want in the k8s.io/api repo, then we can run a script to cut the new repo.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 10, 2017
@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Apr 10, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@caesarxuchao caesarxuchao added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Apr 10, 2017
@caesarxuchao caesarxuchao changed the title Move api helpers.go Move api helpers.go to a subpackage Apr 10, 2017
@caesarxuchao caesarxuchao force-pushed the move-api-helpers.go branch 2 times, most recently from bfe0965 to 2708d2a Compare April 10, 2017 23:22
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 10, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 10, 2017
@caesarxuchao
Copy link
Member Author

@k8s-bot unit test this

@caesarxuchao caesarxuchao mentioned this pull request Apr 11, 2017
func LoadBalancerStatusDeepCopy(lb *LoadBalancerStatus) *LoadBalancerStatus {
c := &LoadBalancerStatus{}
c.Ingress = make([]LoadBalancerIngress, len(lb.Ingress))
func LoadBalancerStatusDeepCopy(lb *api.LoadBalancerStatus) *api.LoadBalancerStatus {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function seems bad, are there users of it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, a few invocations in pkg/controller/service and pkg/proxy.

c.Message, reflect.TypeOf(c.In), c.In, reflect.TypeOf(c.Out),
)
}

const (
// annotation key prefix used to identify non-convertible json paths.
NonConvertibleAnnotationPrefix = "non-convertible.kubernetes.io"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this stay in the api package?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you address this one, I'll lgtm.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -301,7 +302,7 @@ func Convert_extensions_ReplicaSet_to_v1_ReplicationController(in *extensions.Re
if out.Annotations == nil {
out.Annotations = make(map[string]string)
}
out.Annotations[api.NonConvertibleAnnotationPrefix+"/"+fieldErr.Field] = reflect.ValueOf(fieldErr.BadValue).String()
out.Annotations[helper.NonConvertibleAnnotationPrefix+"/"+fieldErr.Field] = reflect.ValueOf(fieldErr.BadValue).String()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to fix in this PR, but we should probably provide a function instead of making everyone do this concatenation.

@lavalamp
Copy link
Member

I have slightly mixed feelings, but overall I think this is a good step.

@lavalamp
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 11, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, lavalamp

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit abd92fa into kubernetes:master Apr 12, 2017
k8s-github-robot pushed a commit that referenced this pull request Apr 14, 2017
Automatic merge from submit-queue (batch tested with PRs 44406, 41543, 44071, 44374, 44299)

Move pkg/api/ref.go to a subpackage

First commit is #44296. (unfortunately, removing that commit results in conflicts)

This PR moves the pkg/api/ref.go to its own subpackage. It's mostly a mechanic move.

I'll send a few more PRs to make the k8s.io/kubernetes/pkg/api package only contains the code we want in the k8s.io/api repo, then we can run a [script](a0015fd#diff-7a2fbb4371972350ee414c6b88aee1c8) to cut the new repo.
k8s-github-robot pushed a commit that referenced this pull request Apr 14, 2017
Automatic merge from submit-queue (batch tested with PRs 44440, 44038, 44302, 44316, 43876)

Move pkg/api/ref.go and pkg/api/resource_helpers.go to subpackages

First two commits are #44296 #44299. (unfortunately, removing these commits results in conflicts)

This PR moves resource_helpers.go to a subpackage. It's mostly a mechanic move, except that:
* i kept the methods of ResourceName and ResourceList in pkg/api/method.go

I'll send one more PR to separate api.Scheme etc. to their own package.
k8s-github-robot pushed a commit that referenced this pull request Apr 15, 2017
Automatic merge from submit-queue (batch tested with PRs 44364, 44361, 42498)

Move v1 helpers

The first 3 commits are other PRs.

This PR move pkg/api/v1/helpers.go to a subpackage, which is almost symmetric to #44296, where pkg/api/helpers.go was moved.

This PR is mostly mechanic, except that
1. moved the 3 methods of Taint and Toleration to pkg/api/methods.go
2. moved constants and types defined in v1/helpers.go to pkg/api/v1/annotataion_key_constants.go and nonstandard_types.go
3. updated staging/copy.sh to copy pkg/api/helpers to client-go, it's otherwise removed from client-go because no other code in client-go depends on the package. Some test code in pkg/controller imports client-go/pkg/api/helpers. After moving api types to its own repo, we can remove these copies of utility function from client-go and ask users to use the ones in the main repo.

(This PR breaks a cyclic import problem i met when I tried to move global variables pkg/api/Scheme and Registry to a subpackage)
dims pushed a commit to dims/kubernetes that referenced this pull request Feb 8, 2018
Automatic merge from submit-queue (batch tested with PRs 44364, 44361, 42498)

Move v1 helpers

The first 3 commits are other PRs.

This PR move pkg/api/v1/helpers.go to a subpackage, which is almost symmetric to kubernetes#44296, where pkg/api/helpers.go was moved.

This PR is mostly mechanic, except that
1. moved the 3 methods of Taint and Toleration to pkg/api/methods.go
2. moved constants and types defined in v1/helpers.go to pkg/api/v1/annotataion_key_constants.go and nonstandard_types.go
3. updated staging/copy.sh to copy pkg/api/helpers to client-go, it's otherwise removed from client-go because no other code in client-go depends on the package. Some test code in pkg/controller imports client-go/pkg/api/helpers. After moving api types to its own repo, we can remove these copies of utility function from client-go and ask users to use the ones in the main repo.

(This PR breaks a cyclic import problem i met when I tried to move global variables pkg/api/Scheme and Registry to a subpackage)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants